fix(Outline, SelectiveBloom): stop unnecessary effect reconstruction and re-runs - #360
Conversation
There was a problem hiding this comment.
🟡 Not ready to approve
The new selection/lights syncing paths can pass nullish ref resolutions into postprocessing APIs, which can cause runtime crashes and should be guarded before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Refactors Outline and SelectiveBloom to avoid unnecessary effect reconstruction and selection-sync re-runs by centralizing selection/layer synchronization into a shared useSelectionSync hook, and adds regression tests to validate the new behavior.
Changes:
- Added
EMPTY_ARRAYand extracted shared selection/layer synchronization intouseSelectionSync. - Updated
OutlineandSelectiveBloomto use the shared hook and stabilize memoization behavior. - Added new Vitest coverage for selection syncing and avoiding unnecessary reconstructions/re-runs.
File summaries
| File | Description |
|---|---|
| src/util.tsx | Adds EMPTY_ARRAY and introduces useSelectionSync for shared selection/layer syncing. |
| src/effects/SelectiveBloom.tsx | Stops unnecessary SelectiveBloomEffect reconstruction and switches to shared selection sync. |
| src/effects/Outline.tsx | Switches Outline’s selection/layer syncing to the shared hook. |
| src/tests/SelectiveBloom.test.tsx | Adds tests for reconstruction avoidance, selection sync behavior, and layer changes. |
| src/tests/Outline.test.tsx | Adds tests to ensure no unnecessary selection resets and correct declarative selection behavior. |
Review details
Suppressed comments (1)
src/effects/SelectiveBloom.tsx:91
- Same as the add loop:
resolveRef(light)can benullfor ref-typed lights, soremoveLightshould also guard against nullish resolved refs to avoid cleanup-time crashes.
lights.forEach((light) => removeLight(resolveRef(light), effect))
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
b272460 to
c30741b
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The wrappers now drop previously-forwarded effect option props (removal of ...props) while their public prop types still advertise broader option support, creating a likely silent API regression.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
src/effects/Outline.tsx:45
- OutlineProps is typed as the full OutlineEffect options object (ConstructorParameters[2]), but the wrapper now only forwards the explicit subset listed here (no
...props). This can silently drop valid OutlineEffect options passed by consumers and is a behavioral/API change from the previous...propsforwarding.
new OutlineEffect(scene, camera, {
blendFunction,
patternTexture,
patternScale,
edgeStrength,
src/effects/SelectiveBloom.tsx:53
- SelectiveBloomProps still accepts all keys from BloomEffectOptions, but the SelectiveBloomEffect constructor options are now hard-coded to the explicit list below (no
...propsforwarding). Any BloomEffectOptions fields not enumerated here will be silently ignored, which is a behavioral/API change from the previous implementation that forwarded...props.
const effect = useMemo(() => {
const instance = new SelectiveBloomEffect(scene, camera, {
blendFunction: BlendFunction.ADD,
luminanceThreshold,
luminanceSmoothing,
mipmapBlur,
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
30e568e to
dae9ece
Compare
…nd re-runs Unstable `[]` prop defaults and `props` in a useMemo dep array caused SelectiveBloomEffect to reconstruct (and both effects' declarative selection to re-set) on every unrelated render. Extracted the shared selection-sync logic into `useSelectionSync`, and fixed an effect ordering bug found along the way (lights were assigned to a stale selection layer).
…effect options reactive Copilot review: resolveRef can return null before a ref attaches, which crashed effect.selection.set()/addLight. Filtered nulls, and fixed ObjectRef's own type to admit null so tsc catches this going forward. Also enumerated the remaining BloomEffectOptions/OutlineEffect options explicitly instead of an unstable ...props spread, so they update reactively instead of only applying on first mount.
dae9ece to
04bc289
Compare
Found while manually verifying #359 with Outline/SelectiveBloom live:
SelectiveBloom'suseMemohadpropsin its deps, so the wholeSelectiveBloomEffectgot reconstructed on every selection change. Separately, both effects defaultedselection/lightsto= [], a fresh array on every render, causing the declarative-selection effect to re-run on every unrelated render, whether or not<Selection>was even in use.Extracted the shared selection-sync logic (declarative array mode + Selection/Select context mode) into a single
useSelectionSynchook, used by both effects - removes the duplication and fixes both bugs in one place.Test plan: 4 new tests (2 per effect) - no reconstruction/re-run on unrelated renders, correct selection sync, and the layer-ordering.